Automatically Profile roots when DevTools is present #13058

bvaughn · 2018-06-16T01:07:50Z

I'm working on a new DevTools plug-in that uses the new profiler timings to create charts about an application's rendering performance. One of these charts will be a flame graph that shows all fibers in an application. The width of each fiber will be based on its treeBaseTime (the time it last took to render) and the color will be based on how much time it took relative to the longest running parts of the commit. Here's a small demo:

Profiling when DevTools are present

My original idea was for DevTools to walk each root and turn ProfileMode on for all fibers when the user started profiling. (PR #12910 was created in order to support this functionality.) I made an oversight though. React does not collect timing information for fibers that aren't in ProfileMode, and so selfBaseTime and treeBaseTime values are all set to 0. This means that when a user starts profiling, base times are all 0. These times get updated when a fiber next renders– but not if it bails out. This means that in some cases, parts of the tree will not have base times and the resulting graph will be inaccurate.

There are a couple of options to address this:

Profiling builds of React always collection timing information if DevTools has been detected. This way the base time information is already present when a user starts profiling. (This has an added benefit of not requiring DevTools to crawl each tree and modify the mode when recording starts.)
DevTools somehow forces a deep re-render (that ignores shouldComponentUpdate) when profiling is started so that all fiber are guaranteed to have a base time value.

This PR adopts the first option, but I'm happy to discuss the second one if there are strong preferences or concerns.

Testing DevTools profiler integration

This PR also updates the test renderer to inject itself into DevTools if the global hook is present. This was done so that I could add a test for the otherwise unobservable profiling behavior. (It may also be useful for other tests.)

I made a few judgement calls here that I don't feel strongly about and would be happy to discuss changing:

Test renderer does not support the findFiberByHostInstance method because I didn't want to add the overhead of mapping instances to fibers until/unless we had a need for that feature.
Test renderer DevTools injection is based on a new supportDevToolsIfPresent feature flag, which is only enabled for the tests that depend on it.
I updated the other DevTools-capable renderers to also check this feature flag because it seemed a bit weird not to. (I particularly don't feel strongly about this change.)

pull-bot · 2018-06-16T01:16:48Z

ReactDOM: size: 0.0%, gzip: -0.0%

Details of bundled changes.

Comparing: 8e87c13...854ef7e

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.1%	+0.1%	623.3 KB	623.65 KB	145.64 KB	145.76 KB	UMD_DEV
react-dom.production.min.js	0.0%	-0.0%	95.18 KB	95.18 KB	30.74 KB	30.73 KB	UMD_PROD
react-dom.development.js	+0.1%	+0.1%	619.43 KB	619.78 KB	144.46 KB	144.58 KB	NODE_DEV
react-dom.production.min.js	0.0%	0.0%	95.17 KB	95.17 KB	30.29 KB	30.29 KB	NODE_PROD
ReactDOM-dev.js	+0.1%	+0.1%	626.68 KB	627.04 KB	143.09 KB	143.21 KB	FB_WWW_DEV
ReactDOM-prod.js	🔺+0.1%	🔺+0.1%	271.76 KB	271.92 KB	51.22 KB	51.25 KB	FB_WWW_PROD
react-dom.profiling.min.js	+0.1%	+0.1%	96.07 KB	96.14 KB	30.59 KB	30.61 KB	NODE_PROFILING

react-art

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-art.development.js	+0.1%	+0.2%	407.58 KB	407.93 KB	91.21 KB	91.35 KB	UMD_DEV
react-art.development.js	+0.1%	+0.2%	339.55 KB	339.91 KB	74.1 KB	74.24 KB	NODE_DEV
ReactART-dev.js	+0.1%	+0.2%	329.62 KB	329.97 KB	69.19 KB	69.33 KB	FB_WWW_DEV
ReactART-prod.js	🔺+0.1%	🔺+0.3%	142.8 KB	143 KB	24.55 KB	24.63 KB	FB_WWW_PROD

react-test-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-test-renderer.development.js	+1.1%	+1.5%	341.82 KB	345.61 KB	74.02 KB	75.11 KB	UMD_DEV
react-test-renderer.production.min.js	🔺+2.5%	🔺+2.5%	47.29 KB	48.46 KB	14.51 KB	14.87 KB	UMD_PROD
react-test-renderer.development.js	+1.1%	+1.5%	337.94 KB	341.73 KB	73.05 KB	74.13 KB	NODE_DEV
react-test-renderer.production.min.js	🔺+2.5%	🔺+2.6%	46.99 KB	48.17 KB	14.32 KB	14.68 KB	NODE_PROD
ReactTestRenderer-dev.js	+1.1%	+1.5%	341.13 KB	345 KB	71.83 KB	72.89 KB	FB_WWW_DEV

react-reconciler

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-reconciler.development.js	+0.1%	+0.2%	330.08 KB	330.43 KB	70.58 KB	70.72 KB	NODE_DEV
react-reconciler.production.min.js	0.0%	-0.0%	47.09 KB	47.09 KB	14.09 KB	14.09 KB	NODE_PROD
react-reconciler-persistent.development.js	+0.1%	+0.2%	328.69 KB	329.04 KB	70.01 KB	70.15 KB	NODE_DEV
react-reconciler-persistent.production.min.js	0.0%	-0.0%	47.11 KB	47.11 KB	14.1 KB	14.09 KB	NODE_PROD

react-native-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
ReactNativeRenderer-dev.js	+0.1%	+0.1%	463.82 KB	464.17 KB	102 KB	102.12 KB	RN_FB_DEV
ReactNativeRenderer-prod.js	🔺+0.1%	🔺+0.1%	206.8 KB	206.96 KB	36.42 KB	36.47 KB	RN_FB_PROD
ReactNativeRenderer-dev.js	+0.1%	+0.1%	463.53 KB	463.88 KB	101.94 KB	102.06 KB	RN_OSS_DEV
ReactNativeRenderer-prod.js	0.0%	0.0%	199.36 KB	199.36 KB	34.96 KB	34.96 KB	RN_OSS_PROD
ReactFabric-dev.js	+0.1%	+0.1%	454.07 KB	454.42 KB	99.61 KB	99.72 KB	RN_FB_DEV
ReactFabric-dev.js	+0.1%	+0.1%	454.11 KB	454.46 KB	99.63 KB	99.74 KB	RN_OSS_DEV
ReactNativeRenderer-profiling.js	+0.1%	+0.1%	201.9 KB	202.03 KB	35.51 KB	35.56 KB	RN_OSS_PROFILING
ReactFabric-profiling.js	+0.1%	+0.1%	193.87 KB	194.01 KB	34.03 KB	34.07 KB	RN_OSS_PROFILING

Generated by 🚫 dangerJS

gaearon · 2018-06-16T01:40:36Z

packages/shared/ReactFeatureFlags.js

@@ -39,6 +39,9 @@ export const warnAboutLegacyContextAPI = false;
 // Gather advanced timing metrics for Profiler subtrees.
 export const enableProfilerTimer = __PROFILE__;

+// If the DevTools hook is present, inject into it.
+export const supportDevToolsIfPresent = false;


Shouldn't this be true? I think this caused devtools code to get DCE'd in open source builds.

sebmarkbage · 2018-06-16T01:51:19Z

packages/react-test-renderer/src/ReactTestRenderer.js

@@ -279,6 +281,10 @@ class ReactTestInstance {
    return this._currentFiber().memoizedProps;
  }

+  get treeBaseTime(): number {


The Devtools are going to use a private api to access this data in a way that we support only because we can push new versions of it so mitigates maintainance burden.

You can use the private api to test this behavior since the private api is the “internally public” api, the one actually used.

Even if we wanted to expose this in the future I don’t think we are ready to do that yet.

Sure, that makes sense.

sebmarkbage · 2018-06-16T01:54:55Z

packages/react-dom/src/client/ReactDOM.js

-          'font-weight:bold',
-        );
+  if (__DEV__) {
+    if (!foundDevTools && canUseDOM && window.top === window.self) {


I didn’t realize we limit it to the top most frame. Seems hacky and unfortunate limitation.

bvaughn · 2018-06-19T14:30:57Z

Ping 😄

gaearon · 2018-06-19T17:17:33Z

packages/shared/forks/ReactFeatureFlags.persistent.js

@@ -21,6 +21,7 @@ export const warnAboutDeprecatedLifecycles = false;
 export const warnAboutLegacyContextAPI = false;
 export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__;
 export const enableProfilerTimer = false;
+export const supportDevToolsIfPresent = false;


gaearon · 2018-06-20T15:36:26Z

What is the utility in making supportDevToolsIfPresent conditional? Can we remove the flag, and always inject?

bvaughn · 2018-06-20T15:41:06Z

We definitely could. I don't feel strongly about this. It just seemed a little odd for the OSS react-test-renderer to have DevTools injection logic, when it doesn't really support or interact with DevTools in a meaningful way.

gaearon · 2018-06-20T15:43:51Z

Does it hurt to have it though? I imagine one might build some interactive React tutorial that embeds the test renderer. Would be cool if DevTools worked with it.

bvaughn · 2018-06-20T15:48:32Z

No, not really.

As the PR description mentions, I made a few judgement calls here that I don't feel strongly about 😄 This is one of them. I'll back it out.

gaearon · 2018-06-20T15:50:22Z

packages/react-reconciler/src/ReactFiber.js

+  let mode = isAsync ? AsyncMode | StrictMode : NoContext;
+
+  if (enableProfilerTimer && isDevToolsPresent) {
+    // Always collect profile timings when DevTools are present.


This only affects the profiling bundle, right?

At first I was a bit worried we'd do this in production mode too, but now I realized that we seemingly don't. Then it should be fine.

In general we try to avoid any extra overhead based on DevTools presence (i.e. until we actually connect). But a bit of overhead in profiling bundle specifically (and not in production) seems okay to me.

Yes, it only affects profiling builds.

gaearon

LGTM assuming my understanding is correct, and any overhead only affects profiling mode bundles.

I'd probably remove supportDevToolsIfPresent. I don't think it gives us something valuable, and each flag adds some maintenance cost and a chance of getting it wrong (as happened in this PR twice 😛 ).

There is some weird artifact with ReactTestRendererComponentTree.js showing up in diff view with no changes. Not sure what that's about.

gaearon · 2018-06-20T15:53:56Z

packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js

+
+    // Measure observable timing using the Profiler component.
+    // The time spent in App (above the Profiler) won't be included in the durations,
+    // But needs to be acocunted for in the offset times.


Had to read 3x to spot it

gaearon · 2018-06-20T15:54:02Z

packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js

+
+    // Measure observable timing using the Profiler component.
+    // The time spent in App (above the Profiler) won't be included in the durations,
+    // But needs to be acocunted for in the offset times.


gaearon · 2018-06-20T15:54:39Z

packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js

+    // At this point, the base time should include both:
+    // The initial 9ms for the components that do not re-render, and
+    // The updated 6ms for the component that does.
+    expect(rendered.root.findByType(App)._currentFiber().treeBaseTime).toBe(15);


Can you explain more about why this is completely unobservable? Is it because it's only used for the UI, and not reported by the Profiler component? Should it be reported by Profiler component somehow?

Maybe "unobservable" isn't the right word to use, but this time value won't be tracked at all unless the DevTools are detected– because App isn't inside of a Profiler root (which is intentional or this test).

My main concern is with using _currentFiber() here which isn't a public API. Is there any way we could write these tests without reaching into the fiber data structure? If not (and if that's how DevTools works anyways) then it's okay.

I don't think so. Also, this is kind of doing what Sebastian suggested (assuming I understood his comment correctly).

DevTools wouldn't be using _currentFiber() but yeah, this seems like the easiest way without bringing the backend itself into this repo. I assume you're reading treeBaseTime from the Fiber in DevTools—can't check because I'm not sure where the code is.

bvaughn · 2018-06-20T15:57:34Z

Thanks for the review, Dan 😄

…n/treeBaseDuration This is an unobservable change to all but the (under development) DevTools Profiler plugin. It is being done so that the plugin can safely feature detect a version of React that supports it. The profiler API has existed since the 16.4.0 release, but it did not support the DevTools plugin prior to PR facebook#13058. Side note: I am not a big fan of the term "base duration". Both it and "actual duration" are kind of awkward and vague. If anyone has suggestions for better names– this is the best time to bikeshed about them.

…n/treeBaseDuration (#13156) This is an unobservable change to all but the (under development) DevTools Profiler plugin. It is being done so that the plugin can safely feature detect a version of React that supports it. The profiler API has existed since the 16.4.0 release, but it did not support the DevTools plugin prior to PR #13058. Side note: I am not a big fan of the term "base duration". Both it and "actual duration" are kind of awkward and vague. If anyone has suggestions for better names– this is the best time to bikeshed about them.

* react-test-renderer injects itself into DevTools if present * Fibers are always opted into ProfileMode if DevTools is present * Added simple test for DevTools + always profiling behavior

facebook-github-bot added the CLA Signed label Jun 16, 2018

bvaughn requested a review from sebmarkbage June 16, 2018 01:30

gaearon reviewed Jun 16, 2018

View reviewed changes

sebmarkbage reviewed Jun 16, 2018

View reviewed changes

Brian Vaughn added 2 commits June 19, 2018 10:16

react-test-renderer injects itself into DevTools if present

e434513

Fibers are always opted into ProfileMode if DevTools is present

6a01fc1

gaearon reviewed Jun 19, 2018

View reviewed changes

Brian Vaughn added 7 commits June 19, 2018 10:17

Added feature flag to control DevTools injection

b2a6d5a

Moved DevTools ProfileMode check to create-host function

a12c0bb

Renamed test description

6e9ee84

Removed unsupported method

6546e32

Added missing feature flag to test

8bfbbe3

supportDevToolsIfPresent true for base feature flags file

7a37500

Remove treeBaseTime getter from test renderer

3e22e59

bvaughn force-pushed the Profiler-DevTools-integration-test branch from 111a4f0 to 3e22e59 Compare June 19, 2018 17:20

supportDevToolsIfPresent = true for persistent config

36151d1

gaearon reviewed Jun 20, 2018

View reviewed changes

gaearon approved these changes Jun 20, 2018

View reviewed changes

Removed supportDevToolsIfPresent feature flag

4a68804

gaearon reviewed Jun 20, 2018

View reviewed changes

Removed unused ReactTestRendererComponentTree

2776f6f

Fixed typo

854ef7e

bvaughn merged commit b0f6089 into facebook:master Jun 20, 2018

bvaughn deleted the Profiler-DevTools-integration-test branch June 20, 2018 16:24

This was referenced Jun 29, 2018

Reset profiler timer correctly after errors #13123

Merged

Better feature detection for DevTools Profiler plug-in #13148

Closed

bvaughn mentioned this pull request Jul 6, 2018

Renamed *BaseTime Fiber attributes to *baseDuration #13156

Merged

gaearon mentioned this pull request Sep 5, 2018

Changelog for 16.5.0 #13571

Merged

guyfedwards mentioned this pull request Sep 7, 2018

Update to react@v16.5.0 weaveworks/ui-components#372

Closed

dependencies bot mentioned this pull request Oct 1, 2018

Update react-dom in /modules/frontend from 16.3.0 to 16.5.2 project-accelerate/accelerate#277

Open

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

This was referenced Dec 1, 2018

Update react-test-renderer in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#312

Open

Update react in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#313

Open

This was referenced Sep 5, 2024

[Snyk] Fix for 104 vulnerabilities Omrisnyk/lock-walker#65

Open

[Snyk] Fix for 104 vulnerabilities Omrisnyk/lock-walker#66

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically Profile roots when DevTools is present #13058

Automatically Profile roots when DevTools is present #13058

bvaughn commented Jun 16, 2018 •

edited

Loading

pull-bot commented Jun 16, 2018 •

edited

Loading

react-dom

react-art

react-test-renderer

react-reconciler

react-native-renderer

gaearon Jun 16, 2018

sebmarkbage Jun 16, 2018

bvaughn Jun 16, 2018

sebmarkbage Jun 16, 2018

bvaughn commented Jun 19, 2018

gaearon Jun 19, 2018

gaearon commented Jun 20, 2018

bvaughn commented Jun 20, 2018

gaearon commented Jun 20, 2018

bvaughn commented Jun 20, 2018

gaearon Jun 20, 2018

bvaughn Jun 20, 2018

gaearon left a comment

gaearon Jun 20, 2018

bvaughn Jun 20, 2018

gaearon Jun 20, 2018

gaearon Jun 20, 2018

bvaughn Jun 20, 2018

gaearon Jun 20, 2018

bvaughn Jun 20, 2018

gaearon Jun 20, 2018

bvaughn commented Jun 20, 2018

Automatically Profile roots when DevTools is present #13058

Automatically Profile roots when DevTools is present #13058

Conversation

bvaughn commented Jun 16, 2018 • edited Loading

Profiling when DevTools are present

Testing DevTools profiler integration

pull-bot commented Jun 16, 2018 • edited Loading

react-dom

react-art

react-test-renderer

react-reconciler

react-native-renderer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Jun 19, 2018

Choose a reason for hiding this comment

gaearon commented Jun 20, 2018

bvaughn commented Jun 20, 2018

gaearon commented Jun 20, 2018

bvaughn commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Jun 20, 2018

bvaughn commented Jun 16, 2018 •

edited

Loading

pull-bot commented Jun 16, 2018 •

edited

Loading